-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add module #1
Add module #1
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also do:
- Add pre-commits (see terraform-vm-azurerm)
ae01eba
to
078e967
Compare
cfbae89
to
1121835
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remote test missing
Signed-off-by: philthoennissen <[email protected]>
Signed-off-by: philthoennissen <[email protected]>
Signed-off-by: philthoennissen <[email protected]>
Signed-off-by: philthoennissen <[email protected]>
Signed-off-by: philthoennissen <[email protected]>
Signed-off-by: Roman Schwarz <[email protected]> Signed-off-by: philthoennissen <[email protected]>
Signed-off-by: Roman Schwarz <[email protected]> Signed-off-by: philthoennissen <[email protected]>
Signed-off-by: philthoennissen <[email protected]>
Signed-off-by: philthoennissen <[email protected]>
Signed-off-by: philthoennissen <[email protected]>
Signed-off-by: philthoennissen <[email protected]>
Signed-off-by: philthoennissen <[email protected]>
Signed-off-by: philthoennissen <[email protected]>
Signed-off-by: philthoennissen <[email protected]>
Signed-off-by: philthoennissen <[email protected]>
10ae4df
to
7d81d66
Compare
Signed-off-by: philthoennissen <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for developing this module! I've added some comments and recommendations in my review. Please take these as constructive feedback aimed at improving the module. They are based on the Terraform Module Style Guide, which is currently under development.
If you'd like, we can discuss the individual points in a meeting.
- Please remove the empty
main.tf
file after splitting resources into multiple files. - Ensure all references to renamed resources and data sources are updated accordingly.
- Rename the
tests/local/mock_providers
directory totests/local/mocks
for consistency. - Please also use the latest module-ci.yaml workflow. This workflow also runs all tests (example, local, remote), and this would fail in this PR.
I did not review the INSTALL.md
for now. I will do this as soon as the module is working.
Signed-off-by: philthoennissen <[email protected]>
Signed-off-by: philthoennissen <[email protected]>
Signed-off-by: philthoennissen <[email protected]>
Signed-off-by: philthoennissen <[email protected]>
Signed-off-by: philthoennissen <[email protected]>
Signed-off-by: philthoennissen <[email protected]>
Quick Fix for several suggestions Co-authored-by: Andre Licht <[email protected]> Signed-off-by: Phil-Thoennissen <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The module is really coming together. Thanks for your work on this! 👍🏻
Here are a few small things that still need to be adjusted.
As a side note, the remote test failed. |
Signed-off-by: philthoennissen <[email protected]>
Co-authored-by: Roman Schwarz <[email protected]> Signed-off-by: Phil-Thoennissen <[email protected]>
Signed-off-by: philthoennissen <[email protected]>
Signed-off-by: philthoennissen <[email protected]>
Signed-off-by: philthoennissen <[email protected]>
@rswrz Everything should be done now and pipelines are green as well. |
Hey @Phil-Thoennissen, I’ve noticed that the Storage Account (also in the upstream Launchpad Terraform code) allows Blob Public Access by default. We should change this. Please set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your work! There are still a few minor issues with the documentation and the primary usage example. Please don't hesitate to reach out if you need any clarification.
Signed-off-by: philthoennissen <[email protected]>
Signed-off-by: philthoennissen <[email protected]>
Signed-off-by: philthoennissen <[email protected]>
Signed-off-by: philthoennissen <[email protected]>
Initital Module.
Please take a closer look at INSTALL.md line 52-60